calibre: Switch to MSI Installer #16785
Conversation
WalkthroughMigrates Calibre from a portable to MSI-based installation, adds pre/post-install and uninstaller scripts for settings/library migration, converts manifest metadata types (license, notes), and adds comprehensive registry scripts to register/unregister file associations and the calibre:// URL protocol. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Scoop as "Scoop"
participant PS as "PowerShell scripts"
participant MSI as "MSI Installer"
participant Registry as "Windows Registry"
participant Calibre as "Calibre App"
rect rgb(240,247,255)
User->>Scoop: scoop install calibre
Scoop->>PS: run pre_install (migrate settings)
PS-->>Scoop: migration complete
end
rect rgb(255,248,230)
Scoop->>MSI: download & run MSI installer
MSI->>Calibre: place files into Program Files
MSI-->>Scoop: install complete
end
rect rgb(246,255,242)
Scoop->>PS: run post_install (migrate libraries, create shortcuts)
PS->>Registry: apply install-associations.reg & register-url-handler.reg
Registry-->>PS: keys/values created
PS-->>Scoop: post-install complete
end
rect rgb(255,240,246)
User->>Calibre: open calibre://... or double-click file
Calibre->>Registry: query protocol/file association
Registry-->>Calibre: return command (calibre.exe "%1")
Calibre->>Calibre: launch with argument
end
rect rgb(248,248,248)
User->>Scoop: scoop uninstall calibre
Scoop->>PS: run uninstaller (unregister associations, cleanup)
PS->>Registry: apply uninstall-associations.reg & unregister-url-handler.reg
Registry-->>PS: keys/values removed
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
All changes look good. Wait for review from human collaborators. calibre-normal
calibre
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
bucket/calibre.json (1)
96-113: Consider simplifying the main shortcut name.Line 99 creates a shortcut path "Calibre - E-book Management\Calibre - E-book Management", resulting in a redundant name. Consider shortening the shortcut name to just "Calibre" since it's already within the "Calibre - E-book Management" folder:
[ "Calibre\\calibre.exe", - "Calibre - E-book Management\\Calibre - E-book Management" + "Calibre - E-book Management\\Calibre" ],This would make it consistent with the other shortcuts (E-Book Editor, E-Book Viewer, LRF Viewer) which don't repeat "Calibre" in their names.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
bucket/calibre-normal.json(1 hunks)bucket/calibre.json(2 hunks)scripts/calibre/register-url-handler.reg(1 hunks)scripts/calibre/unregister-file-associations.reg(1 hunks)scripts/calibre/unregister-url-handler.reg(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: WindowsPowerShell
🔇 Additional comments (13)
scripts/calibre/unregister-file-associations.reg (1)
3-3: LGTM: Calibre settings cleanup on uninstall.Removing the main calibre registry key during uninstall is appropriate for cleanup.
bucket/calibre.json (8)
19-32: Review migration logic and conditional flow.The pre_install migration has several considerations:
Lines 21-22: The conditional checks if settings exist OR if env var already points to persist_dir. The second condition appears to be the expected state (env var should point to persist_dir), so returning early when it's already configured correctly is logical.
Lines 23-27: Migrates from custom CALIBRE_CONFIG_DIRECTORY and returns, preventing fallthrough to APPDATA migration. This prioritization seems correct—honor user's explicit env var over default location.
Lines 28-31: Falls back to migrating from default APPDATA location only if previous conditions weren't met.
The logic appears sound for handling multiple migration scenarios (fresh install, env var user, default APPDATA user).
34-45: LGTM: Installer cleanup logic handles migration scenarios.The installer script properly:
- Identifies and removes conflicting configuration directories (symlinks from calibre-normal, custom env var locations)
- Extracts the MSI content to the expected location
- Uses conditional array building (lines 40, 42) to only include directories that need cleanup
This cleanup prevents conflicts when migrating from calibre-normal's symlink approach.
47-54: LGTM: Registry template processing with proper escaping and global install handling.The post_install script correctly:
- Escapes backslashes for Windows registry format (line 48)
- Processes all .reg templates from the scripts directory
- Conditionally replaces HKEY_CURRENT_USER with HKEY_LOCAL_MACHINE for global installs, except for the unregister script which should always target the current user
- Substitutes the calibre installation path into templates
71-75: LGTM: Environment variables provide portable-like behavior.The environment variables correctly point Calibre to persisted directories, achieving portable-like functionality with the MSI installer as described in the PR objectives.
76-95: LGTM: Bin entries updated for MSI installer layout.The bin entries correctly reflect the MSI installer's directory structure with the Calibre\ subdirectory prefix.
118-125: LGTM: Uninstaller properly cleans up registry entries.The uninstaller correctly:
- Checks for uninstall command to avoid running during upgrades
- Removes URL handler registration
- Removes file associations
This addresses the PR objective of cleaning up residual registry entries.
129-138: LGTM: Autoupdate configured for MSI installer.The autoupdate section correctly uses the MSI URL format, consistent with the manual URL definition.
9-12: The manual URL handler registration instruction is necessary and correctly documented. Thepost_installscript (lines 49-54) only generates and prepares the registry file with template substitution; it does not automatically import it. The script copiesregister-url-handler.regfrom the extras/scripts directory to$dirbut stops there. Users must manually execute thereg importcommand to activate the URL protocol handler. This pattern is consistent with the uninstaller (lines 121-122), which explicitly imports the unregister registry files.bucket/calibre-normal.json (2)
2-15: LGTM: Clear deprecation notice with migration path.The manifest properly communicates:
- Deprecation timeline (2026-12-01)
- Benefits of migrating to extras/calibre
- Automatic data migration
- Resolution of URL protocol and file association issues
This gives users ample time to transition.
16-76: LGTM: Simplified manifest appropriate for deprecated package.This deprecated manifest provides basic MSI extraction without the migration scripts, portable behavior, or registry cleanup. This is appropriate as users are encouraged to migrate to the main calibre manifest.
scripts/calibre/unregister-url-handler.reg (1)
1-3: LGTM: URL handler unregistration is correct.The registry script properly removes the calibre URL protocol registration by deleting the protocol key.
scripts/calibre/register-url-handler.reg (1)
3-15: LGTM: URL protocol registration follows Windows conventions.The registry script correctly implements Windows URL protocol registration with:
- Protocol identifier and description (lines 3-5)
- Icon specification using template variable (lines 7-8)
- Shell command association with URL argument placeholder (lines 10-15)
The template variable
{{calibre_dir}}will be substituted during post_install (bucket/calibre.json line 53), resolving issue #12659.
|
/verify |
|
All changes look good. Wait for review from human collaborators. calibre-normal
calibre
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
bucket/calibre.json (1)
49-53: Silent failure if registry scripts directory doesn't exist.Lines 49–53 process registry templates from
$bucketsdir\\extras\\scripts\\$app, but the directory path is never verified to exist. If the directory is missing,Get-ChildItemwill silently fail (due to line 36's$ErrorActionPreference = 'SilentlyContinue'), and registry files won't be created in$dir. This means URL handlers and file associations won't be registered.Consider adding a check or warning if no registry scripts are found:
$registryScripts = @(Get-ChildItem -Path "$bucketsdir\\extras\\scripts\\$app" -Filter '*.reg' -File -ErrorAction SilentlyContinue) if ($registryScripts.Count -eq 0) { Write-Host "WARNING: No registry scripts found for URL handler registration" -ForegroundColor Yellow return } $registryScripts | ForEach-Object { ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
bucket/calibre-normal.json(1 hunks)bucket/calibre.json(2 hunks)scripts/calibre/register-url-handler.reg(1 hunks)scripts/calibre/unregister-file-associations.reg(1 hunks)scripts/calibre/unregister-url-handler.reg(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- bucket/calibre-normal.json
- scripts/calibre/unregister-file-associations.reg
- scripts/calibre/register-url-handler.reg
- scripts/calibre/unregister-url-handler.reg
🔇 Additional comments (9)
bucket/calibre.json (9)
1-11: Metadata updates aligned with MSI migration.License change from
GPL-3.0-onlytoGPL-3.0-or-laterwith HEAD reference, and notes about URL protocol registration are appropriate additions. Description formatting is consistent.
19-31: Configuration migration logic is sound.The
pre_installscript correctly handles three migration scenarios:
- Early return if Calibre Settings already exist
- Priority to
CALIBRE_CONFIG_DIRECTORYenvironment variable if set- Fallback to
%APPDATA%\calibrefor standard installationsThis aligns with the PR objectives for supporting environment variable–based portability.
55-69: Library migration logic maintains data integrity.The logic correctly:
- Checks if new location is empty and source has content (line 62) before attempting migration
- Copies library data (line 64)
- Updates configuration (line 67–68)
As clarified in prior review feedback, the unconditional removal on line 66 is safe because the early return on line 61 ensures paths are different, and the condition on line 62 ensures the source has content while the destination is empty.
One edge case: if Copy-Item fails silently due to global
$ErrorActionPreference = 'SilentlyContinue'(line 36), the Remove-Item will still execute. However, this is a rare scenario and Scoop's use of silent error suppression is standard practice.Consider verifying the Copy-Item success explicitly to prevent data loss in edge cases:
if (-not (Test-Path "$persist_dir\\Calibre Library\\*\") -and (Test-Path "$($cfg.library_path)\\*")) { Write-Host \"`nINFO Migrating Calibre Library to \"$persist_dir\\Calibre Library\"...\" -ForegroundColor darkgray Copy-Item -Path "$($cfg.library_path)\\*" -Destination "$persist_dir\\Calibre Library" -Force -Recurse + if (Test-Path "$persist_dir\\Calibre Library\\*\") { Remove-Item -Path $cfg.library_path -Force -Recurse -ErrorAction SilentlyContinue + } else { + Write-Host "ERROR: Library migration failed, old location preserved" -ForegroundColor Red + } }
71-75: Environment variables enable portable-like behavior.The
env_setblock correctly defines the three environment variables needed to support portable-style behavior with MSI installations:
CALIBRE_TEMP_DIR,CALIBRE_CACHE_DIRECTORY, andCALIBRE_CONFIG_DIRECTORYall point to the persistent user data directoryThis satisfies the PR objectives for supporting portable-like configurations.
76-94: Binary paths updated for MSI layout.All binaries correctly point to the
Calibre\subdirectory (created by line 44's MSI extraction). The addition ofcalibredb.exe(line 82) aligns with the MSI package contents. Path consistency is maintained across all entries.
96-112: Shortcuts refactored for consistent menu structure.Menu items reorganized under the "Calibre - E-book Management" category. All four primary shortcuts (main app, editor, viewer, LRF viewer) are present with updated paths. Structure is clean and user-friendly.
118-125: Uninstaller cleanly removes URL handlers and file associations.The uninstaller script imports two registry files to clean up:
unregister-url-handler.reg(removes calibre:// protocol handler)unregister-file-associations.reg(removes e-book file type associations)This addresses PR objective #12659 (URL protocol issues) by ensuring a clean uninstall. The conditional
if ($cmd -eq 'uninstall')guard is appropriate.
129-137: Autoupdate URL pattern correctly reflects MSI format.The URL pattern
https://download.calibre-ebook.com/$version/calibre-64bit-$version.msi#/dl.msi_matches the initial installer URL (line 15) and correctly extracts the MSI with the#/dl.msi_fragment. Hash lookup fromcalibre-ebook.com/signaturesis appropriate.
44-44: No action required. TheExpand-MsiArchivefunction with-ExtractDirand-Removalparameters is a standard Scoop utility. Both parameters are confirmed as supported and properly used in the calibre.json manifest, consistent with their usage across multiple official Scoop Extras manifests including sonic-pi.json, powertoys.json, enpass.json, and boinc.json.
|
/verify |
|
All changes look good. Wait for review from human collaborators. calibre-normal
calibre
|
|
/verify |
|
All changes look good. Wait for review from human collaborators. calibre-normal
calibre
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
scripts/calibre/unregister-file-associations.reg (1)
356-359: Consider adding a trailing newline.The file ends abruptly after line 359. While most registry import tools handle this gracefully, adding a trailing blank line ensures compatibility with all tools and follows common convention for text files.
[-HKEY_CURRENT_USER\Software\Classes\calibre64bit.AssocFile.XPS] +bucket/calibre.json (1)
36-43: Clarify the array multiplication pattern for maintainability.The pattern
@($value) * $is_differenton lines 40 and 42 is clever but non-obvious. When$is_differentis$false(coerced to 0), it produces an empty array, effectively filtering out values.$link_target_dir = @($config_link_target_dir) * $is_differentThis works but may confuse future maintainers. A brief inline comment or using a more explicit conditional would improve readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
bucket/calibre-normal.json(1 hunks)bucket/calibre.json(2 hunks)scripts/calibre/register-url-handler.reg(1 hunks)scripts/calibre/unregister-file-associations.reg(1 hunks)scripts/calibre/unregister-url-handler.reg(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/calibre/unregister-url-handler.reg
- scripts/calibre/register-url-handler.reg
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-13T09:37:06.093Z
Learnt from: o-l-a-v
Repo: ScoopInstaller/Extras PR: 16328
File: bucket/winutil.json:0-0
Timestamp: 2025-10-13T09:37:06.093Z
Learning: When reviewing code in Scoop manifests, always verify the actual file content if the diff appears incomplete or unusual, as diff context can be truncated or displayed incorrectly. The pattern `[string[]](...).ForEach{...}` is commonly used in post_uninstall scripts across the Extras repository.
Applied to files:
bucket/calibre.json
📚 Learning: 2025-10-15T11:54:31.320Z
Learnt from: o-l-a-v
Repo: ScoopInstaller/Extras PR: 16341
File: bucket/foxit-pdf-reader.json:47-50
Timestamp: 2025-10-15T11:54:31.320Z
Learning: In bucket/foxit-pdf-reader.json and bucket/foxit-reader.json, the checkver script uses MaximumRedirection 1 (not 0) for Invoke-WebRequest to properly handle Foxit's backend redirect behavior when parsing the Location header for version detection.
Applied to files:
bucket/calibre.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: WindowsPowerShell
🔇 Additional comments (7)
bucket/calibre-normal.json (2)
10-15: Well-structured deprecation notes with clear migration guidance.The notes provide helpful context for users, explaining the MSI switch, automatic data migration, and the deprecation timeline. The inline reference to
extras/calibreis appropriate for this repository.
6-9: License metadata properly structured.Using the SPDX identifier
GPL-3.0-or-laterwith a direct link to the LICENSE file follows best practices for Scoop manifest metadata.scripts/calibre/unregister-file-associations.reg (1)
1-3: Clean registry cleanup script structure.The script properly deletes the calibre software key and systematically removes all file associations. The casing for
OpenWithProgIDsis now consistent throughout the file.bucket/calibre.json (4)
9-12: Manual URL protocol registration may confuse users.The notes require users to manually run a
reg importcommand to register the URL protocol handler. This contrasts with the PR objective to "fix missing or broken protocol/file-association registration" automatically.Consider whether this could be automated in
post_installfor a better user experience, or if there's a specific reason (like requiring elevation) that necessitates manual execution.
47-54: Backslash escaping is correct for registry format.The
-replace '\\\\', '\\\\'in JSON translates to-replace '\\', '\\'in PowerShell after JSON parsing, which correctly doubles single backslashes for registry path format. This was confirmed working in previous review discussions.
55-69: Library migration logic is sound.The flow correctly handles:
- Fresh install (creates new
global.py.jsonwith default library path)- Existing config with matching path (early return on line 61)
- Migration from different location (copies if target empty, source has content)
The unconditional removal on line 66 is intentional per the author's explanation—under normal usage, both locations cannot simultaneously contain library files.
118-125: Uninstaller script handles cleanup correctly.The
$cmd -eq 'uninstall'check ensures registry cleanup only runs during actual uninstallation, not during updates. Redirecting output with*> $nullsilences both success and error messages appropriately.
|
/verify |
|
All changes look good. Wait for review from human collaborators. calibre
|
|
/verify |
|
All changes look good. Wait for review from human collaborators. calibre-normal
calibre
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
bucket/calibre.json (1)
45-45: Fix the backslash escaping pattern.The current pattern
-replace '\\\\', '\\\\'looks for two consecutive backslashes in the path, but string interpolation"$dir\\Calibre"produces single backslashes. This makes the replacement a no-op, leaving paths with single backslashes that won't work correctly in registry files expecting double backslashes.Based on the previous discussion, the correct pattern is
-replace '\\', '\\'as demonstrated by SorYoshino's test.🔎 Proposed fix
-$calibre_dir = "$dir\\Calibre" -replace '\\\\', '\\\\' +$calibre_dir = "$dir\\Calibre" -replace '\\', '\\'
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
bucket/calibre-normal.jsonbucket/calibre.json
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-13T09:37:06.093Z
Learnt from: o-l-a-v
Repo: ScoopInstaller/Extras PR: 16328
File: bucket/winutil.json:0-0
Timestamp: 2025-10-13T09:37:06.093Z
Learning: When reviewing code in Scoop manifests, always verify the actual file content if the diff appears incomplete or unusual, as diff context can be truncated or displayed incorrectly. The pattern `[string[]](...).ForEach{...}` is commonly used in post_uninstall scripts across the Extras repository.
Applied to files:
bucket/calibre.json
📚 Learning: 2025-10-15T11:54:31.320Z
Learnt from: o-l-a-v
Repo: ScoopInstaller/Extras PR: 16341
File: bucket/foxit-pdf-reader.json:47-50
Timestamp: 2025-10-15T11:54:31.320Z
Learning: In bucket/foxit-pdf-reader.json and bucket/foxit-reader.json, the checkver script uses MaximumRedirection 1 (not 0) for Invoke-WebRequest to properly handle Foxit's backend redirect behavior when parsing the Location header for version detection.
Applied to files:
bucket/calibre.json
🔇 Additional comments (7)
bucket/calibre-normal.json (1)
2-15: LGTM! Metadata updates align with deprecation strategy.The deprecation notice, license object format, and notes array provide clear guidance for users migrating to the MSI-based
extras/calibremanifest. The notes effectively communicate the migration timeline and benefits.bucket/calibre.json (6)
3-14: LGTM! Metadata properly updated for MSI installer.The license format, description, and notes array appropriately document the MSI-based installation workflow and provide clear instructions for registry operations.
21-29: LGTM! Configuration migration logic is sound.The pre_install script correctly handles configuration directory migration with appropriate checks to avoid overwriting existing settings.
31-42: LGTM! Installer cleanup logic handles migration edge cases.The conditional array multiplication pattern correctly handles cleanup of old configuration directories and symlinks before extracting the MSI archive.
68-73: LGTM! Environment variables properly configured for portable-like behavior.The environment variables correctly point to persisted directories, providing the portable functionality described in the PR objectives.
74-111: LGTM! Bin and shortcuts properly updated for MSI layout.The paths and shortcut names correctly reflect the MSI installation structure with the
Calibresubdirectory.
127-136: LGTM! Autoupdate configuration matches MSI URL format.The autoupdate block correctly uses MSI-based URLs and hash verification.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
bucket/calibre.json (2)
45-45: Fix backslash escaping pattern for registry file generation.The current pattern
-replace '\\\\', '\\\\'is a no-op because it matches two consecutive backslashes and replaces them with two backslashes. SinceJoin-Pathreturns paths with single backslashes (e.g.,C:\Users\test\Calibre), this pattern won't match anything, and the registry templates will receive paths with single backslashes instead of the required double backslashes.The author's own demonstration in past comments showed the correct pattern is
-replace '\\', '\\', which doubles each single backslash.🔎 Proposed fix
- "$calibre_dir = \"$dir\\Calibre\" -replace '\\\\', '\\\\'", + "$calibre_dir = \"$dir\\Calibre\" -replace '\\', '\\'",
119-119: Fix registry filename to match actual unregister file.Line 119 references
uninstall-associations.reg, but:
- The PR description specifies
unregister-file-associations.regis copied to$dir- Line 120 uses the
unregister-prefix pattern for URL handlers- Past comments identified this issue, and while it was partially fixed (changed from
install-associations.reg), the filename is still incorrectThe filename should be
unregister-file-associations.regfor consistency with the naming pattern and to match the actual file copied by the post_install script.🔎 Proposed fix
- " reg import \"$dir\\uninstall-associations.reg\" *> $null", + " reg import \"$dir\\unregister-file-associations.reg\" *> $null",
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bucket/calibre.json
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-13T09:37:06.093Z
Learnt from: o-l-a-v
Repo: ScoopInstaller/Extras PR: 16328
File: bucket/winutil.json:0-0
Timestamp: 2025-10-13T09:37:06.093Z
Learning: When reviewing code in Scoop manifests, always verify the actual file content if the diff appears incomplete or unusual, as diff context can be truncated or displayed incorrectly. The pattern `[string[]](...).ForEach{...}` is commonly used in post_uninstall scripts across the Extras repository.
Applied to files:
bucket/calibre.json
📚 Learning: 2025-10-15T11:54:31.320Z
Learnt from: o-l-a-v
Repo: ScoopInstaller/Extras PR: 16341
File: bucket/foxit-pdf-reader.json:47-50
Timestamp: 2025-10-15T11:54:31.320Z
Learning: In bucket/foxit-pdf-reader.json and bucket/foxit-reader.json, the checkver script uses MaximumRedirection 1 (not 0) for Invoke-WebRequest to properly handle Foxit's backend redirect behavior when parsing the Location header for version detection.
Applied to files:
bucket/calibre.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: WindowsPowerShell
🔇 Additional comments (9)
bucket/calibre.json (9)
21-29: LGTM! Pre-install migration logic is well-structured.The script properly:
- Ensures the persistent settings directory exists
- Skips migration if settings are already present
- Checks standard config locations (environment variable or AppData)
- Migrates existing configurations when found
30-42: LGTM! Installer cleanup and MSI extraction are correct.The script appropriately:
- Removes legacy config directories (default, symlink targets, environment-based) before installation
- Extracts the MSI to the correct destination with proper path structure
46-50: LGTM! Registry file templating logic is correct.The script properly:
- Iterates through all
.regfiles in the scripts directory- Handles global installs by remapping registry hives
- Substitutes the calibre_dir path template variable
- Saves processed files with Unicode encoding (required for
.regfiles)
51-67: LGTM! Settings and library migration logic is sound.The script correctly:
- Ensures temporary and cache directories exist
- Creates default configuration if needed
- Migrates library content when appropriate (non-empty source, empty target)
- Updates configuration to point to new library location
- Uses error suppression for safe cleanup of old paths
Based on the author's prior clarification, the logic correctly handles migration scenarios.
68-73: LGTM! Environment variables correctly support portable-like behavior.The variables appropriately:
- Disable default program registration (avoiding system-wide changes)
- Point temp, cache, and config to persistent directories
- Align with the persistent directory structure defined in the manifest
74-111: LGTM! Bin and shortcuts correctly reflect MSI structure.The updates properly:
- Adjust all executable paths to the MSI-based
Calibre\directory structure- Organize shortcuts under "Calibre - E-book Management" folder
- Use clear, descriptive names for shortcuts
112-115: LGTM! Persist directories are correctly configured.The persistent directories match those created and referenced throughout the installation and configuration scripts.
124-136: LGTM! Checkver and autoupdate correctly configured for MSI.The configuration properly:
- Uses GitHub as the version source
- Updates the URL pattern to MSI format
- Retrieves hash verification from the official source
46-50: Registry filename references in manifest are correct.Verification confirms all registry files referenced in the manifest (lines 11, 13, 119, 120) exist in
scripts/calibre/and match the actual filenames:install-associations.reg,register-url-handler.reg,uninstall-associations.reg, andunregister-url-handler.reg. The post_install script correctly copies all.regfiles from the scripts directory.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
bucket/calibre-normal.json (1)
6-8: Revert the license identifier toGPL-3.0-only.As flagged in a previous review, Calibre is licensed under GNU GPL v3 only (not "or later"). The FSF Free Software Directory explicitly lists
GPL-3.0-onlyfor Calibre.🔎 Proposed fix
"license": { - "identifier": "GPL-3.0-or-later", + "identifier": "GPL-3.0-only", "url": "https://github.com/kovidgoyal/calibre/blob/HEAD/LICENSE" },bucket/calibre.json (2)
6-7: Revert the license identifier toGPL-3.0-only.Same issue as in
calibre-normal.json— Calibre is licensed under GNU GPL v3 only, not "or later".🔎 Proposed fix
"license": { - "identifier": "GPL-3.0-or-later", + "identifier": "GPL-3.0-only", "url": "https://github.com/kovidgoyal/calibre/blob/HEAD/LICENSE" },
45-45: Fix the path construction and backslash escaping.The current approach has issues:
"$dir\\Calibre"uses string concatenation with literal\\which creates inconsistent paths (single backslash from$direxpansion + double backslash literal)- The
-replace '\\\\', '\\\\'is a no-op (replaces 2 backslashes with 2 backslashes)As discussed in the previous review, use
Join-Pathfor proper path construction and-replace '\\', '\\'for escaping:🔎 Proposed fix
- "$calibre_dir = \"$dir\\Calibre\" -replace '\\\\\\\\', '\\\\\\\\'", + "$calibre_dir = (Join-Path $dir 'Calibre') -replace '\\\\', '\\\\'",
🧹 Nitpick comments (1)
bucket/calibre.json (1)
59-65: Verify library migration safety in edge cases.Line 63's
Remove-Itemexecutes unconditionally after the migration check. If migration is skipped because the target already has content (line 59), the old library location is still deleted. While the author confirmed this is acceptable for normal usage, consider moving the removal inside the migration block for defensive safety:🔎 Proposed safer approach
" if (-not (Test-Path -Path \"$persist_dir\\Calibre Library\\*\") -and (Test-Path -Path \"$($cfg.library_path)\\*\")) {", " Write-Host \"`nINFO Migrating Calibre Library to '$persist_dir\\Calibre Library'...\" -ForegroundColor DarkGray", " Copy-Item -Path \"$($cfg.library_path)\\*\" -Destination \"$persist_dir\\Calibre Library\" -Force -Recurse", + " Remove-Item -Path $cfg.library_path -Recurse -Force -ErrorAction SilentlyContinue", " }", - " Remove-Item -Path $cfg.library_path -Recurse -Force -ErrorAction SilentlyContinue",
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
bucket/calibre-normal.jsonbucket/calibre.jsonscripts/calibre/install-associations.regscripts/calibre/register-url-handler.regscripts/calibre/uninstall-associations.regscripts/calibre/unregister-url-handler.reg
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/calibre/unregister-url-handler.reg
- scripts/calibre/register-url-handler.reg
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-13T09:37:06.093Z
Learnt from: o-l-a-v
Repo: ScoopInstaller/Extras PR: 16328
File: bucket/winutil.json:0-0
Timestamp: 2025-10-13T09:37:06.093Z
Learning: When reviewing code in Scoop manifests, always verify the actual file content if the diff appears incomplete or unusual, as diff context can be truncated or displayed incorrectly. The pattern `[string[]](...).ForEach{...}` is commonly used in post_uninstall scripts across the Extras repository.
Applied to files:
bucket/calibre.json
📚 Learning: 2025-10-15T11:54:31.320Z
Learnt from: o-l-a-v
Repo: ScoopInstaller/Extras PR: 16341
File: bucket/foxit-pdf-reader.json:47-50
Timestamp: 2025-10-15T11:54:31.320Z
Learning: In bucket/foxit-pdf-reader.json and bucket/foxit-reader.json, the checkver script uses MaximumRedirection 1 (not 0) for Invoke-WebRequest to properly handle Foxit's backend redirect behavior when parsing the Location header for version detection.
Applied to files:
bucket/calibre.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: WindowsPowerShell
🔇 Additional comments (6)
bucket/calibre-normal.json (1)
2-15: Deprecation notice and migration notes are well-structured.The deprecation comment, updated notes array, and clear guidance for users to migrate to
extras/calibreare appropriate. The notes effectively communicate the MSI switch, automatic data migration, and deprecation timeline.scripts/calibre/install-associations.reg (1)
1-1476: File associations registry template is well-structured.The template correctly:
- Uses
{{calibre_dir}}placeholder for path substitution- Registers
OpenWithProgIDswithhex(0):(empty REG_NONE value) per Windows convention- Creates proper shell command handlers for each file type
- Maps extensions to appropriate executables (calibre.exe, ebook-viewer.exe, ebook-edit.exe)
The HKCU scope is appropriate for Scoop's per-user installation model.
bucket/calibre.json (3)
116-123: Uninstaller logic is correct.The uninstaller properly imports the registry cleanup files (
uninstall-associations.regandunregister-url-handler.reg) and correctly guards execution with$cmd -eq 'uninstall'. This addresses the issue flagged in a previous review.
21-28: Pre-install configuration migration logic is sound.The script correctly:
- Ensures the persist directory exists
- Short-circuits if config already migrated
- Checks both
CALIBRE_CONFIG_DIRECTORYenv var and default APPDATA location- Migrates existing configuration with clear user feedback
68-73: Environment variables properly configure portable-like behavior.Setting
CALIBRE_NO_DEFAULT_PROGRAMS=1prevents automatic file association registration, allowing users to opt-in via manual registry import as documented in the notes. The temp, cache, and config directories are correctly pointed to persistent locations.scripts/calibre/uninstall-associations.reg (1)
1-357: Uninstall registry file correctly removes all Calibre file associations.The script properly deletes both the OpenWithProgIDs registry values and the corresponding class key definitions for all 40 file extensions. Registry syntax is correct—removing values with
"name"=-and deleting keys with[-HKEY_PATH]will recursively delete all subkeys. Extension coverage matches install-associations.reg with no omissions.
|
/verify |
|
All changes look good. Wait for review from human collaborators. calibre-normal
calibre
|
1 participant
Summary
Switches
calibreto the MSI installer for improved maintainability, adds automatic user data migration, environment variables for portable-like behavior, and registers URL protocol handlers. Markscalibre-normalas deprecated.Related issues or pull requests
Changes
calibre-normalafter 2026-12-01calibrefrom portable installer to MSI installerregister-url-handler.regfor registering thecalibre:protocolpre_installandpost_installscripts to migrate user configuration and libraryCALIBRE_TEMP_DIR,CALIBRE_CACHE_DIRECTORY,CALIBRE_CONFIG_DIRECTORYbinandshortcutsfields for the MSI structureuninstallerscript to remove URL handlers and file associationscheckverandautoupdateto use MSI-based URLsNotes
calibrenow resolves issues with unregistered URL protocols and residual file associations after uninstallationThe
notesregarding the discontinued 32-bit version were added to the manifest in 2022, and it has now been three years, so I have not re-included them in thenotesfield. The relevant output is already sufficient.The tests described in this PR are not comprehensive and only cover the installation of
calibre-normal. Data migration tests are divided into the following four scenarios. Click the corresponding links to view the testing methods and results.calibrecalibreinstallation to the MSI Installer version ofcalibrecalibre-normaltocalibre- Symbolic Linkscalibre-normaltocalibre- Environment VariablesWhen the
windows_register_default_programsfield ingui.jsonis missing or its value does not match the currently installed version, runningcalibre.exewill automatically register file associations, writing the relevant registry entries toHKCU.Therefore, this PR does not provide a script to register file associations. At the same time,unregister-file-associations.regonly needs to be copied to$dir.To ensure a one-to-one correspondence between registered file association entries and
$global, the environment variableCALIBRE_NO_DEFAULT_PROGRAMSis used to prevent the application from automatically registering related file associations. Theinstall-associations.regfile is then provided so that users can decide whether to register the file associations manually.Testing
The test results are as follows:
<manifest-name[@version]|chore>: <general summary of the pull request>Summary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.